Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maya: Use resolution and frame range from task entity #39

Conversation

bradenjennings
Copy link
Contributor

@bradenjennings bradenjennings commented Feb 12, 2024

Changelog Description

AYON supports task frame range and resolution overrides. Lets validate against the task override if available, and repair to the task override.

Additional info

Paragraphs of text giving context of additional technical information or code examples.

Testing notes:

  1. start with this step
  2. follow this step

cuID:AY-1052

@ynbot ynbot added type: enhancement Improvement of existing functionality or minor addition size/XS labels Feb 12, 2024
@bradenjennings
Copy link
Contributor Author

Why did one of the GitHub checks fail?

@iLLiCiTiT iLLiCiTiT changed the title enhancement/OP-7955_Context_Validation_Repair_Action_enhancement Maya: Use scene resolution from task entity Feb 12, 2024
@bradenjennings bradenjennings changed the title Maya: Use scene resolution from task entity Maya: Use resolution and frame range from task entity Feb 12, 2024
Comment on lines 56 to 75
# Get frame information from task entity
# NOTE: If there is no task override then the asset
# value is automatically returned instead
task_entity = instance.context.data["taskEntity"]
frame_start_handle = task_entity["attrib"]["frameStart"] - \
task_entity["attrib"]["handleStart"]
frame_end_handle = task_entity["attrib"]["frameEnd"] + \
task_entity["attrib"]["handleEnd"]
handle_start = task_entity["attrib"]["handleStart"]
handle_end = task_entity["attrib"]["handleEnd"]
frame_start = task_entity["attrib"]["frameStart"]
frame_end = task_entity["attrib"]["frameEnd"]

# Get frame information from asset context
# frame_start_handle = int(context.data.get("frameStartHandle"))
# frame_end_handle = int(context.data.get("frameEndHandle"))
# handle_start = int(context.data.get("handleStart"))
# handle_end = int(context.data.get("handleEnd"))
# frame_start = int(context.data.get("frameStart"))
# frame_end = int(context.data.get("frameEnd"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to query this data in a separate class method so the logic isn't duplicated between here and repair

E.g.

        def get_instance_task_frame_range(instance):
            # Get frame information from task entity
            # NOTE: If there is no task override then the asset
            # value is automatically returned instead
            attrib = instance.context.data["taskEntity"]["attrib"]
            handle_start = attrib["handleStart"]
            handle_end = attrib["handleEnd"]
            frame_start = attrib["frameStart"]
            frame_end = attrib["frameEnd"]
            frame_start_handle = frame_start - handle_start
            frame_end_handle = frame_end + handle_end

We could potentially even improve the readability by having a FrameRange dataclass.
For example:

@dataclasses.dataclass
class FrameRange:
    """Frame range with handles.

    The frame range excludes the handles - and thus the handles are outside
    of the frame range. To get the inclusive start and end frame use
    `frame_start_handle` and `frame_end_handle`.

    """
    frame_start: int
    frame_end: int
    handle_start: int
    handle_end: int

    @property
    def frame_start_handle(self) -> int:
        return self.frame_start - self.handle_start

    @property
    def frame_end_handle(self) -> int:
        return self.frame_end + self.handle_end


# Example usage
frames = FrameRange(frame_start=1001,
                    frame_end=1010,
                    handle_start=5,
                    handle_end=10)

print(frames.frame_start_handle)

Then the method becomes:

        def get_instance_task_frame_range(instance):
            # Get frame information from task entity
            # NOTE: If there is no task override then the asset
            # value is automatically returned instead
            attrib = instance.context.data["taskEntity"]["attrib"]
            return FrameRange(
                frame_start=attrib["frameStart"],
                frame_end=attrib["frameEnd"],
                handle_start=attrib["handleStart"],
                handle_end=attrib["handleEnd"],
            )

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore I still think this should check the frame range against the entity of the instance - not the current asset so that you still validate to the asset you're publishing to, not where you're publishing from.

So likely we'll also need to collect the task entity for the instance - and use that one here instead.

Copy link
Contributor Author

@bradenjennings bradenjennings Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the get_instance_task_frame_range, and FrameRange object as suggested in the validate_frame_range.py and pushed the changes now.

I'm not sure what you mean by this below. The validation and repair action seem to be doing the right thing in my testing so far.

Furthermore I still think this should check the frame range against the entity of the instance - not the current asset so that you still validate to the asset you're publishing to, not where you're publishing from.

So likely we'll also need to collect the task entity for the instance - and use that one here instead.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create two instances in your scene, change folder + task on one of the instances via the publisher UI.
Your one scene, now publishes into two different folder + task entities.

The frame range validation should now validate the frame range of the selected task entity - not from your current context. So make sure in the database that each folder (and its task?) entity has a different frame range set so you can confirm the validation adheres to that actual selected task entity's frame range.

The current logic relies on instance.context of which during publishing there's only one - and thus I'm quite confident this currently does not work the way I describe.

Copy link
Contributor Author

@bradenjennings bradenjennings Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigRoy is it just a matter of storing taskEntity in the instance data. so it gets stored per instance, rather than per context.
instance.data["taskEntity"]

however assetEntity is collected in collect_context_entities.py, so wouldn't it make sense to collect the taskEntity in the same place?
Once per context.

Copy link
Contributor Author

@bradenjennings bradenjennings Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigRoy I updated the collection of taskEntity to happen within collect_instances.py rather than collect_context_entities.py,
https://github.com/ynput/ayon-core/pull/39/files#diff-13c18dd7cb684126254981e14bcf1590c1c5be889bc327ce05bbddc404d2133fR96

Is this the right approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigRoy do you have any further information about approach above?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it just a matter of storing taskEntity in the instance data. so it gets stored per instance, rather than per context.
instance.data["taskEntity"]

Yes, along those lines.

Basically the same behavior as is done for "assets" here basically inheriting the asset entity from context if it matches the context otherwise querying the differing asset entity instead.

Copy link
Contributor Author

@bradenjennings bradenjennings Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigRoy sorry I do not follow.

is it possible for you to propose a code change for this?
It's really difficult to follow in written form.

Or is my code already basically doing the right thing.

@tokejepsen
Copy link
Member

Could we get some testing steps for this?

@BigRoy
Copy link
Collaborator

BigRoy commented Feb 13, 2024

@tokejepsen I suspect it should be something like:

Additional info

The frame range validation will validate whether the instance's frame range and handles matches that of the folder+task entity in the AYON database.

If the task does not specify a frame range (and/or handles) override than the task entity should inherit it from the parent folder by default.

Testing steps

  1. Create two folders with different frame ranges and handles
  2. Create two folders, where on the tasks you have specified different frame range and handle overrides.
  3. From one Maya workfile, create 4 instances, each set to one of the four folder/tasks.

During validation now each instance should be validated against its own corresponding frame range as set in AYON.
The repair functionality of the validator should also work to update the instance.

Note that the Validator only runs over families:

    families = ["animation",
                "pointcache",
                "camera",
                "proxyAbc",
                "renderlayer",
                "review",
                "yeticache"]

So make sure to test with any of those instances.

As always, "renderlayer" is special because it does not specify the frame range on the instance but on the renderlayer itself. As such, I'd do the above testing steps once with "camera" instances and with e.g. "renderlayer" instances to ensure both work.

Maya also has the "attachTo" functionality where a subset, like pointcacheMain can be added into the review instances. It might be worth also checking that particular case - to see how the publishes behave if e.g. a pointcacheMain instances is added into the review instance.

Also test "Reset Frame Range" on the AYON menu in Maya - it should be updated as well.

Another thing to potentially test is that in some cases a instance may not have a Task specified - e.g. tray publisher can publish without selecting a task for some publish types. It might be good to also test whether the task entity collecting in the global collector does not break those cases. I'm not sure if Maya in particular has any Creator types that allow to create without a task. I'm not entirely sure when this is the case - but @iLLiCiTiT can likely elaborate.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 13, 2024

Please guys, keep in mind that ayon-core still has a lot of OpenPype compatibility. And something like taskEntity on instance.data and stuff like that should be done in completelly different PR with completelly different scope.

We are still in phase 2 mentioned here https://community.ynput.io/t/openpype-to-ayon-core-addon-conversion/1209/2 . Don't try to hurry things up.

We can afford to do this singular change (if it's client related), otherwise it's a big breaker. I have prepared many other PRs which are waiting for merging of existing PRs. Any changes related to entities and stuff around will break all my work I've already did.

So if this won't work because the data are validated and we would need the same information to be used during publishing, then I would recommend to put this On Hold and wait until release 1.0.0 is out.

@tokejepsen
Copy link
Member

@iLLiCiTiT can we continue this PR now?

@iLLiCiTiT
Copy link
Member

After this PR #165

@antirotor
Copy link
Member

#165 is merged, we can continue

@moonyuet
Copy link
Member

moonyuet commented Apr 5, 2024

#165 is merged, we can continue

@antirotor
This issue has been resolved bit by bit through different PRs, and some of them are already merged.
#340 for validate instances in same context by me(Still on review)
#358 for validate resolution by @BigRoy (Merged)
#338 for validate frame range by @MustafaJafar (Merged)
Guess this ticket can be closed after #340?

@moonyuet
Copy link
Member

Closed this PR as the issue has been resolved with #340 , #338 , #358

@moonyuet moonyuet closed this Apr 10, 2024
@moonyuet moonyuet deleted the enhancement/OP-7955_Context_Validation_Repair_Action_enhancement branch April 10, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Maya size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants